It seems to me that it would be simpler and more robust to have this integrated into constructMagicVariables() rather than as a list maintained speparately. We'll need to remember to keep the list in sync when stuff is added to or removed from stdlib and it's easy to forget or make a mistake.

I see that GlobalContext is constructed at the beginning of the analysis but already after parsing. At this point we already know if we're in experimental mode so we could have a constructor parameter that sets a flag, which constructMagicVariables() would use to not include some of the magic variables.

This would also simplify the analysis because for example we would not need any special-casing for stdlib in DeclarationContainer to prevent these magic variables from getting resolved.

I haven't read through the code myself so far - but why exactly do we need this list anyways? The point in making these things stdlib-defined is that they're no longer magic variables...

Without looking at it, is this due to the rest of analysis breaking if it has declarations for magic variables? We'll likely swap out those parts of analysis soon anyways, so :-)... Hm... although name and type resolution would of course be nice to keep around at first, it's currently a mess of an implementation, but it works and will probably keep working for quite a while...

As far as I could tell from review, it's to remove the names provided by stdlib from global scope when experimental mode is enabled. Otherwise you could still use the corresponding global builtins.

We only really need it if we want to make the global built-ins unavailable gradually, as we add them to stdlib. We could instead make them all unavailable right away. Not sure if that's acceptable though - they include stuff like abi or type(), which won't make it into stdlib for quite some time.

Exactly; I think making them unavailable now is the way to go (which makes the current implementation wrong, as you can still use the builtins even if you don't import from the standard library; in fact, I think it's even worse - these are implicitly imported from the standard library even if you don't explicitly import them, which makes this even worse). I think in general, this should be reworked prior to merging, even though the current implementation does work, if only by luck :). The list in GlobalContext should go as well. When I say I want to make the global builtins unavailable now - I don't actually mean all of them; rather, after parsing is completed (and we do parse the standard library files as well), we already know the names of exposed standard library functions, which means we can exclude only those from the global builtins, (if that makes sense).

This test does not seem to be adding anything meaningful to what the other three test for these functions already do. Maybe it would be better to remove it?

By the way, is this little coverage really adequate for stdlib? I know the functions are simple and all, but still, it's the standard library. I'm assuming that what we have here is all we'll have, because I can't imagine anyone will have the patience to come back later and add proper tests for these things. This never happens in practice.

I'd prefer if we at least had some mechanism to test that the new functions give the same outputs as the existing built-ins on a bigger range or inputs, especially including some corner cases.

The test is here only to cover different import mechanisms, i.e. import "std/cryptography.sol" vs import { sha256 } from "std/cryptography.sol"`, I can remove it if it's superfluous.

By the way, is this little coverage really adequate for stdlib?

No, these were just some sanity cases I added to the previous PR, since that one had only one test originally if I remember correctly.

I'd prefer if we at least had some mechanism to test that the new functions give the same outputs as the existing built-ins

I agree, although I'm not really sure how to go about this, since if pragma experimental solidity is enabled, we shouldn't have access to the builtins (even though currently we do), which means we'd either have to have this as a unit test where we compile one source unit that uses builtins, and the other that uses stdlib, preserve the return values, and then compare them. soltest/isoltest doesn't allow this, as far as I know.

The test is here only to cover different import mechanisms,

Ah, ok then. I did not notice that. I guess it's fine then, but I'd recommend changing its name to make it more obvious.

I agree, although I'm not really sure how to go about this

Well, we might build something specifically for this later (e.g. allow using current builtins in isoltest expectations), but for now I'd be satisfied simply with more coverage. We already have some tests for these 3 builtins, we could replicate them for experimental too.

Why a command-line test? It seems to me like this would work perfectly fine as a syntax test. Is this maybe a left-over from some initial version that needed CLI flags to enable stdlib or something?

No, there was never a cmdline flag to enable the standard library, it was always done via a pragma. This was a test case from the original PR, and the failed import throws a compiler error from CompilerStack, so I just assumed this couldn't be handled by a syntax test; I'd argue this isn't really a syntax test either, since the syntax itself is correct? In any case, I can try creating a syntax test for this and see how it works out?

Our syntax tests cover parsing too so that should not be a problem.

If it throws from CompilerStack, then yeah, that's the reason. I expected these to be properly caught by isoltest but maybe they're not. I now see it does not even have a proper error code.

In that case I'd question if we really want this to be reported this way. It does not have a location, even though we could easily provide one, and will drive users nuts :) I'm even wondering if this will not be reported as an ICE in Standard JSON. Well, maybe this is fine for the experimental version. It's something that will have to be ironed out sooner or later, before we make it non-experimental, but that's still way off in the future.

This function does not clean v.

Internal functions do not automatically clean their arguments so if there's anything in the higher bits of the value someone passes into this function, it will show when you access it in inline assembly and will affect the calculations. The other arguments are fine because they take up whole slots and cannot have dirty bits. v can.

That is, unless the precompile itself ignores those higher bits. This may or may not be the case, not sure - but in any case we should definitely have a test case covering this. And if it does ignore them I'd at least expect a comment pointing this out because it's not obvious.

This, I had no idea about, and it's what I'm currently looking into by comparing it with the IR code generator, but I don't really see anything special here compared to the other precompiles.

The version that's currently in the codegen uses ABI encoder on the arguments, which takes care of the problem. Yours does not do that. Which is fine, using ABI encoding just for this seems like an overkill, but you still do need manual cleanup in that case.

See the cleanup example from my operator blog post: https://blog.soliditylang.org/2023/02/22/user-defined-operators/#parameter-cleanup. You should be able to create a repro for this problem by using ecrecover() the same way I use yoloDiv() there (note that this does not depend on operators and UDVTs in any way, you can replace them with free function calls and uint8).

Let me know if you need any help with fixing this or creating a test, I can explain everything.